Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

POC: Transition compatibility hooks #44151

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

devknoll
Copy link

@devknoll devknoll commented Oct 18, 2024

Proof of concept of compatibility hooks enabling transitions to be used in React 17 applications. There are a few key differences outlined below.

Compatibility Hooks

unstable_useDeferredValueCompat(value)

Implements React.useDeferredValue. Note that the React 19 initialValue? argument is not currently supported, as there are open questions as to shimming this behavior in React 18.

unstable_useTransitionCompat()

Implements React.useTransition, including support for async actions. Note that the semantics for this hook differ slightly from React. Consider the following:

const [isPending, startTransition] = useTransition();
const [state, dispatch] = useReducer((s, a) => [...s, a], []);

// ...

startTransition(() => {
  dispatch('foo');
});
dispatch('bar');

It's expected that the value of state will be ['foo', 'bar']. That's because conceptually, state updates are queued, and even though the first update was scheduled non-blocking, the update still needs to be applied chronologically before the second update. Unfortunately, this type of queuing is not trivial to implement generally (i.e. for all state) without brittle monkey patching of React.

Instead, the compatibility hook works a little differently. Instead of marking all state updates inside of startTransition as non-blocking, it marks everything as blocking by default -- even in React 18 and higher. To mark state updates as non-blocking with the compatibility hook, the state hook itself must use either unstable_useStateCompat or unstable_useReducerCompat. That means in order to reproduce the React 18 behavior, you need to write this instead:

const [isPending, startTransition] = unstable_useTransitionCompat();
const [state, dispatch] = unstable_useReducerCompat((s, a) => [...s, a], []);

// ...

startTransition(() => {
  dispatch('foo');
});
dispatch('bar');

Once ready to adopt React 18, you would first migrate unstable_useTransitionCompat to useTransition, and then unstable_useStateCompat/unstable_useReducerCompat alternatives.

unstable_useStateCompat

Just like useState, but for use with unstable_useTransitionCompat. If you're not updating the value in a transition, you should just use useState.

unstable_useReducerCompat

Just like useReducer, but for use with unstable_useTransitionCompat. If you're not updating the value in a transition, you should just use useReducer.

How it works

The CompatTransitionManager maintains batches, queues of updates that should be applied together. The primary question is when the batches should be applied:

  1. Whenever a blocking update is queued (i.e. outside of startTransition or useDeferredValue), all previous batches (even non-blocking) must be applied before the blocking update is. This is implemented by blockingBatchCallback

  2. Whenever a non-blocking update is queued, the update can be applied at some point in the future. This is implemented by nonBlockingBatchCallback, which simply uses setTimeout to apply the changes in a future macrotask, so blocking updates have a chance to be painted first.

  3. Running the compatibility hooks in React 18 is a special case, as we want to preserve the same semantics for those upgrading from React 17. This is implemented by createPassthroughBatchCallback, which executes only updates batched via the compatibility hooks inside the actual startTransition.

React has a much more sophisticated strategy than these compatibility hooks. However, without being able to interrupt or reprioritize rendering work, it just doesn't make much sense to be more complex than this. We accept that without incremental rendering, non-blocking rendering will inevitably make the UI less responsive (that's why it was marked as non-blocking in the first place: so it wouldn't make the UI less responsive), and so we just make sure we paint any pending states/loading indicators in a blocking update, and cram as much work as possible into the subsequent update.

Why?

Transitions help make applications more responsive and improve Core Web Vitals like INP, especially on lower end devices like mobile. While the compatibility hooks aren't perfect, they can significantly improve the user experience.

Consider the Autocomplete component. Without transitions, running with 20x slowdown, you end up with something like this:

Screen.Recording.2024-10-16.at.11.35.42.AM.mov

The UI simply freezes while the large list is being rendered. However, by using useDeferredValue, we can achieve this instead:

Screen.Recording.2024-10-16.at.11.26.18.AM.mov

Even in React 17 via unstable_useDeferredValue we can achieve a significant improvement to INP (~800ms vs 2s). While the UI is unable to quickly respond to subsequent interactions while the dropdown content is rendering (due to the lack of time slicing/incremental rendering prior to React 18), we are still able to show our animation and a loading indicator to the user.

(Note: the changes to autocomplete are not implemented in this PR. In reality, we would likely want a browser controlled animation that only starts after e.g. a 400ms delay, so that users on faster devices don't see it).

Ideally, applications would just be able to use React 18 or higher. However, in cases where that's not feasible, they should still ideally use transition and action semantics and APIs.

@devknoll devknoll force-pushed the x-add-transition-compat-hooks branch 7 times, most recently from 8e67af4 to b324cfe Compare October 24, 2024 18:14
@devknoll
Copy link
Author

@mnajdova (cc @romgrk, @Janpot) this should be decent enough for eyes now.

I added an example for one way this might be integrated into the Autocomplete component as a separate commit. Here, the <Fade> component is used so that the loading state will only appear if the non-blocking update takes longer than 400ms to appear. Importantly, it relies on CSS animations so that it still appears even if the main thread is blocked. Likely, you would want to use a similar strategy for other non-trivial built-in components such as the Date Picker, the DataGrid, and potentially even menus and drawers.

For more primitive components, the right answer isn't as clear cut. As a general rule of thumb, actions (and transitions) should be used for interactions that might make the UI unresponsive and/or trigger undesirable loading states (e.g. by causing existing suspense boundaries to revert to their fallback). It would be impossible for a framework like MUI to make this determination on behalf of its users. Therefore, my recommendation would be to:

  1. Identify components where MUI and/or Material Design could feasibly (though not necessarily currently) support rendering a useful pending state in response to an interaction
    • Good examples: Button, Fab, et al after LoadingButton. Tabs, BottomNavigation
    • Poor examples: Switch, Checkbox
  2. Supplement those components with an optional action prop, and suggest its use in the documentation where it makes sense, referring to the low-level handler as more of an escape hatch.
    • E.g. pressAction for buttons, selectAction for tabs

For other components, I would simply strive to help users understand how and when to use transitions and the related APIs by including examples of their use in the documentation, and generally educating them that they should attempt to minimize the amount of work done directly in low-level event handlers like onClick (e.g. avoiding heavy computations, blocking React state updates) in order to keep the UI responsive.

@devknoll devknoll force-pushed the x-add-transition-compat-hooks branch from b324cfe to 26a4109 Compare October 30, 2024 17:14
@@ -24,6 +24,8 @@ import { useDefaultProps } from '../DefaultPropsProvider';
import autocompleteClasses, { getAutocompleteUtilityClass } from './autocompleteClasses';
import capitalize from '../utils/capitalize';
import useSlot from '../utils/useSlot';
import { unstable_useDeferredValueCompat } from '@mui/utils';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
import { unstable_useDeferredValueCompat } from '@mui/utils';
import useDeferredValueCompat from '@mui/utils/useDeferredValueCompat';

And tbh I'd even drop the "compat" from the name. The fact that it's a compat shim is an implementation detail, just like use-sync-external-store is; there's no need to carry that information where the hook is used.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think of the useReducer/useState case? I can drop the Compat suffix there too, but then it's not clear that it's related (and necessary). Maybe useStateWithTransitions? 🤷‍♂️ Let me know your thoughts!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I prefer carrying that semantic rather than "compat".

@@ -422,6 +424,7 @@ const AutocompleteGroupUl = styled('ul', {

export { createFilterOptions };

const EMPTY_ARRAY = [];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really wish the spec would add something like Array.EMPTY, we keep adding empty values like this all over the place.

Copy link
Member

@Janpot Janpot Dec 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we keep adding empty values like this all over the place.

We could add it in our utils package? Not sure there would be a noticeable benefit though 🙂.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure there would be a noticeable benefit though

It avoids useless memory allocations, and in some situations it does make an impact. A lot of the perf PRs I did in the last months were just removing memory allocations.

We could add it in our utils package

I usually do that, and it's frequent in other codebases as well, e.g. https://github.com/styled-components/styled-components/blob/main/packages/styled-components/src/utils/empties.ts

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sure you'll notice a difference avoiding allocations in frequently called code. but we're talking about a single allocation at module scope in a few modules vs. allocating once and importing it. The code a bundler generates to import it from a utils package is probably more overhead than just allocating it in the module itself. I wouldn't be surprised if that styled-components snippet you share is actually slower than what we have here. Your proposal to make it part of the language makes a lot more sense to me.

Copy link
Contributor

@romgrk romgrk Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code a bundler generates to import it from a utils package is probably more overhead than just allocating it in the module itself.

Depends on the bundler, some of them generate a single scope where they just give unique names to all variables including imports/exports so "importing" something in prod is basically just using a variable (IIRC, vite does it), while others do a require-like snippet of code to replace imports (IIRC, webpack).

@romgrk
Copy link
Contributor

romgrk commented Oct 31, 2024

LGTM overall, though I've skimmed through it because I haven't touched this part of the codebase yet.

Do you have a showcase of how it improved Autocomplete? Note that you can get a codesandbox with @mui/material linked to this PR's version of the package in the ci/codesandbox link in the GHA checks.

Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general the proposal looks good to me. I left some minor comments. I would like to test a bit more the Autocomplete component's changes.

@@ -0,0 +1,104 @@
import * as ReactDOM from 'react-dom';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very clean implementations 👌

* TODO: Improve typing.
*/
function useReducerCompat<T>(reducer: any, initializerArg: any, initializer?: any) {
if (typeof React.startTransition === 'function') {
Copy link
Member

@mnajdova mnajdova Oct 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can likely do this condition outside of the hook and just return the regular React.useReducer if the startTransition function exists in React.

Copy link
Author

@devknoll devknoll Oct 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really sure, to be honest 🫤 Consider:

startTransition(() => {
  dispatch('foo');
});
dispatch('bar');

If we use React.useReducer directly even in React 18, then the behavior of dispatch could change significantly depending the installed version of React. Specifically, state (or reducers) that would produce blocking updates in React 17 (because they don't use the shim, and we can't queue updates that don't use the shim) would suddenly produce non-blocking updates in React 18 (because React can queue them).

By using the shim even in React 18, we ensure that the behavior is always the same regardless of the React version: shimmed useTransition will only ever cause shimmed useState/useReducer updates to be non-blocking. Non-shimmed useState/useReducer updates are always blocking, even if changed inside of a shimmed useTransition in React 18: instead, you have to intentionally switch to the non-shimmed useTransition to change the behavior.

At least, those are my thoughts. If you feel particularly strongly about it though, we can change it.

* Like `useState`, but for use with `useTransitionCompat`.
*/
function useStateCompat<T>(initialValue: T | (() => T)): [T, SetStateFn<T>] {
if (typeof React.startTransition === 'function') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as the previous comment

@devknoll devknoll force-pushed the x-add-transition-compat-hooks branch 2 times, most recently from 2ebae08 to 52d57ce Compare October 31, 2024 18:55
@devknoll devknoll force-pushed the x-add-transition-compat-hooks branch from 52d57ce to 052caf8 Compare November 1, 2024 14:08
@@ -24,6 +24,8 @@ import { useDefaultProps } from '../DefaultPropsProvider';
import autocompleteClasses, { getAutocompleteUtilityClass } from './autocompleteClasses';
import capitalize from '../utils/capitalize';
import useSlot from '../utils/useSlot';
import { useDeferredValue } from '@mui/utils';
import Fade from '../Fade';
Copy link
Member

@oliviertassinari oliviertassinari Nov 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be inconsistent with the Select animation, no?

Suggested change
import Fade from '../Fade';

I guess it's also outside of the scope of the problem we go after?

* If a batch doesn't already exist, a new one will be created, and the given
* callback will be executed when it ends.
*/
export function runWithBatch<T>(fn: () => T, batchCallback: BatchCallback): T {
Copy link
Member

@oliviertassinari oliviertassinari Nov 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we make transitions opt-in? Something that only React 19 users can adopt, and hence have no ponyfill?

Suggested change
export function runWithBatch<T>(fn: () => T, batchCallback: BatchCallback): T {

@@ -572,6 +575,9 @@ const Autocomplete = React.forwardRef(function Autocomplete(inProps, ref) {
className: classes.paper,
});

const groupedOptionsDeferred = useDeferredValue(popupOpen ? groupedOptions : EMPTY_ARRAY);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be in the hook version, no? So "Base UI" users can have the same benefit

Suggested change
const groupedOptionsDeferred = useDeferredValue(popupOpen ? groupedOptions : EMPTY_ARRAY);

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: out-of-date The pull request has merge conflicts and can't be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants